fix: safely interpolate variables inside rules regex literals - #1889
fix: safely interpolate variables inside rules regex literals#1889bbrala wants to merge 3 commits into
Conversation
A rule whose regex literal contains a variable, e.g.
`$PHP_VERSION =~ '/^$TARGET_PHP$/'`, aborted the whole pipeline parse.
evaluateRuleIf expands every $VAR with JSON.stringify (correct for ==
operands), but the same pass also quoted a $VAR sitting inside a /regex/
literal, leaking `"` into the pattern and producing an invalid
RE2JS.compile("^\"8.3"$", 0) that throws in eval.
Add a pre-pass that expands $VAR (and \$VAR) raw inside /regex/ literals
on the RHS of =~ / !~ before the general quoting pass, so no variable
survives inside a literal regex. Plain == operands, bare /regex/ with no
variable, and "RHS is a variable holding a regex" are unaffected.
Also corrects the issue firecow#350 test assertion, which had frozen the buggy
behaviour (when: never); the branch regex now matches as GitLab does.
There was a problem hiding this comment.
1 issue found across 3 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
A value substituted into a /regex/ literal on the RHS of =~ / !~ was spliced raw. A value carrying regex metacharacters could break out of the literal: a branch name like `feat/x` closed the pattern early and aborted the whole rules evaluation, and a crafted value (e.g. `zzz/ || true || /x`) could inject expression syntax into the eval. Regex-escape the substituted value so it is matched literally and stays contained. Adds edge-case tests for slash-in-value, injection, literal metacharacter matching, and !~ negation.
The pre-pass rewrote `$$` to a single `$`, which the later global expansion pass then read as `$VAR` and expanded, defeating the escape — a regression versus the previous single-pass behavior. Return `$$` so the global unescape pass handles it as before and the following name is left literal. Adds a regression test.
|
Good catch by cubic — confirmed and fixed in 71dddce. The bug was real: the pre-pass rewrote The suggested remedy (dropping Instead I kept the |
| return escapeRegExp(envs[var1 || var2] ?? ""); | ||
| }, | ||
| ); | ||
| return `${op}${pre}/${expandedPattern}/`; |
There was a problem hiding this comment.
Unset var → // → reaches eval as "anything" =~ // and aborts the run. Master returns false.
| return `${op}${pre}/${expandedPattern}/`; | |
| return `${op}${pre}/${expandedPattern || "(?:)"}/`; |
Still false (matchRE2JS skips zero-length matches, global.ts:21).
| const escapeRegExp = (value: string): string => value.replace(/[.*+?^${}()|[\]\\/]/g, "\\$&"); | ||
| const regexLiteralRhs = /(?<op>=~|!~)(?<pre>\s*["']?)\/(?<pattern>(?:\\.|[^/\\])*)\//g; | ||
| evalStr = evalStr.replaceAll(regexLiteralRhs, (_match, op, pre, pattern) => { | ||
| const expandedPattern = pattern.replaceAll( |
There was a problem hiding this comment.
Escaping breaks regex fragments: /^($ALLOWED)$/ with a|b is false here, true on GitLab and inconsistent with $TAG =~ $TAG_REGEX, which stays unescaped.
| return binary; | ||
| }; | ||
|
|
||
| // A `$VAR` that sits *inside* a regex literal on the RHS of `=~`/`!~` |
There was a problem hiding this comment.
Repo doesn't carry source comments
=~against a regex literal containing a variable crashes the parseA rule like this aborts the entire pipeline parse:
With
PHP_VERSIONandTARGET_PHPset to8.3, gitlab-ci-local builds this JavaScript:The pattern is
^\"8.3"$. Those quotes don't belong there. The injected"closes the JS string literal early,evalthrows, and the whole file fails to load. One rule takes down the run.A note on GitLab semantics
GitLab itself does not expand variables inside a regex literal — the docs say so plainly: "Variables in a regular expression are not expanded." gitlab-ci-local does expand them, and has for a long time (see the
$CI_COMMIT_BRANCH =~ /$BRANCHNAME/case in #350). This PR doesn't change that decision — it keeps the existing expand-inside-regex behavior and makes it safe. If the project would rather drop the expansion to match GitLab exactly, that's a separate, larger change and a maintainer call; happy to take that direction instead.Root cause
Utils.evaluateRuleIfinsrc/utils.tsexpands every$VARwithJSON.stringify(value):That's correct for
==operands —8.3 == 8.3has to become"8.3" == "8.3". But the same global pass also rewrites a$VARsitting inside a/regex/literal, and the quotes meant for the string comparison leak into the pattern. The expander can't tell whether a value lands next to a==or inside a regex.The fix
A pre-pass expands
$VAR(and\$VAR) inside a/regex/on the RHS of=~/!~before the general quoting pass, so no variable survives into a literal regex for that pass to quote. The substituted value is regex-escaped, so it matches literally and can't break out of the literal or inject anything:/^$TARGET_PHP$/withTARGET_PHP=8.3compiles to^8\.3$and matches8.3.$CI_COMMIT_BRANCH =~ /^$BRANCH$/,BRANCH=feat/x— no longer closes the literal early. Before this PR it threw; a slash in a branch name is completely ordinary.zzz/ || true || /xcan no longer inject expression syntax into the eval.The other shapes are untouched:
==operands still get quoted.=~with a literal/regex/and no variable is unchanged.=~where the RHS is a variable that holds a regex ($TAG =~ $TAG_REGEX) still routes through the existing path.Tests
Added cases to
tests/rules-regex.test.ts: a variable interpolated into the pattern, a value containing/(matches literally, no throw), an injection attempt that must stayfalse, literal metacharacter matching, and!~negation with a slash value.One existing assertion changed. The #350 case (
$CI_COMMIT_BRANCH =~ /$BRANCHNAME/) assertedwhen: "never"— that froze the old buggy behavior. WithBRANCHNAME=mastermatchingmaster, it'swhen: "manual"; the test asserts that now, with a comment.Known limitation (not addressed here)
A backslash inside a value is still mishandled on the left side of the match —
JSON.stringify(value).replaceAll("\\\\", "\\")turnsa\binto the JS escapea\b(a backspace), so a literal self-match returnsfalse. That's a pre-existing quirk in the general LHS expansion, unrelated to regex interpolation, and changing it touches every backslash-bearing value — out of scope for this fix.tsc --noEmitandeslintare clean; the rules suites pass (71 tests).Fixes #1893